[Taken]Fix issue #783 & #764 by jmecn · Pull Request #805 · jMonkeyEngine/jmonkeyengine

jmecn · 2018-01-14T10:13:31Z

Fix issue #783 & #764

shamanDevel · 2018-01-14T11:58:33Z

+        store.x = (FastMath.nextRandomFloat() * 2f - 1f) * radius;
+        store.y = (FastMath.nextRandomFloat() * 2f - 1f) * radius;
+        store.z = (FastMath.nextRandomFloat() * 2f - 1f) * radius;
+        store.addLocal(center);


Now it creates a random point within a box of the desired side length (=radius) around the center.
The Do-while loop is still necessary to skip the samples outside the sphere

mhhh... wouldn't it be simpler to do;

store.x = FastMath.nextRandomFloat(); store.y = FastMath.nextRandomFloat(); store.z = FastMath.nextRandomFloat(); store.normalizeLocal(); // <- random direction store.multLocal(radius); // <- mult by the radius to be inside the sphere radius store.addLocal(center); // <- relocate around the center.

No loop needed and the normalize ensure that the point is inside the sphere.

It would be simpler, but would not be uniformly distributed.

Uh yeah first the x2-1 should stay to allow negative values. But then just take à random length between 0 and radius, and you break the uniformity.

I might be wrong, but I think there is needs to more probability mass on the points with bigger radius than on the inner points. So if you normalize and then scale with a value between 0 and radius, that distribution must not be uniform, but something like u'=sqrt(1-u^2), as TehLeo suggested.

TehLeo · 2018-01-14T12:20:27Z

Yes, just as shaman pointed out, it got to pick an uniform point on a sphere. The current method, picks a random point in a box, repeatedly until the point is in the sphere. It forgets to add the center.

Here are two methods:

 do {
    store.x = (FastMath.nextRandomFloat() * 2f - 1f);
    store.y = (FastMath.nextRandomFloat() * 2f - 1f);
    store.z = (FastMath.nextRandomFloat() * 2f - 1f);
} while (store.dot(store) > 1);
store.multLocal(radius);
store.addLocal(center);

Or a method without a while loop. (ref: https://math.stackexchange.com/questions/87230/picking-random-points-in-the-volume-of-sphere-with-uniform-probability )

float l = FastMath.pow(FastMath.nextRandomFloat(), 1f/3f);
float u = FastMath.nextRandomFloat()*2f-1f;
float o = FastMath.nextRandomFloat()*FastMath.TWO_PI;
		
store.z = radius*l*u;
u = FastMath.sqrt(1f-u*u);
		
store.x = radius*l*u*FastMath.cos(o);
store.y = radius*l*u*FastMath.sin(o);
store.addLocal(center);

Nehon

getRandomrPoint must be changed.

As a rule of thumb, for the future, I prefer you do one separate PR for each issue. It's a lot simpler to review.

Nehon · 2018-01-14T12:21:59Z

+        store.x = (FastMath.nextRandomFloat() * 2f - 1f) * radius;
+        store.y = (FastMath.nextRandomFloat() * 2f - 1f) * radius;
+        store.z = (FastMath.nextRandomFloat() * 2f - 1f) * radius;
+        store.addLocal(center);


mhhh... wouldn't it be simpler to do;

store.x = FastMath.nextRandomFloat(); store.y = FastMath.nextRandomFloat(); store.z = FastMath.nextRandomFloat(); store.normalizeLocal(); // <- random direction store.multLocal(radius); // <- mult by the radius to be inside the sphere radius store.addLocal(center); // <- relocate around the center.

No loop needed and the normalize ensure that the point is inside the sphere.

jmecn · 2018-01-14T13:58:41Z

I see. Next time I will make separated PR.
Just modified the code, please check it.

TehLeo · 2018-01-14T14:07:52Z

I've checked your changes. It creates random points within a sphere volume.
However, the points are not uniformly distributed.

See the two methods I posted in the comment above.

jmecn · 2018-01-14T14:15:53Z

ok, let me see..

TehLeo · 2018-01-14T17:57:48Z

Almost there. Except the line:

store.multLocal(FastMath.nextRandomFloat() * radius);

I have to say you are really trying to change the code I provided, which is fine, but try to understand the problem as well.

The random point was already selected uniformly within unit sphere volume within the while loop.
What is left to do is multiply it by radius and add the center.

Thus change that line back to.
store.multLocal(radius);

jmecn · 2018-01-15T03:20:23Z

Oh my.. I finally understand that line now.

Nehon · 2018-01-15T07:29:35Z

@TehLeo good for you?

TehLeo · 2018-01-15T12:23:02Z

Yes, all is fine with #764 .

Nehon · 2018-01-15T19:06:18Z

@jmecn Could you please rebase -i your PR so that it's only 2 commits one for each issue?
Do you know how to use rebase -i?

jmecn · 2018-01-16T02:21:27Z

@Nehon never use rebase before. But I think I can have a try.

Nehon · 2018-01-16T07:21:41Z

Here is a nice an thorough explanation https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Edit: that's why I prefer 1 issue per PR because Github allows me to squash all your commits into one.
But for this PR I'd rather have 2 commits, one for each issue. So you have to clean up yourself ;)

Fix issue #783

Fix infinity loop in EmitterSphereShape. issue #764 I test on both method: public void getRandomPoint1(Vector3f store) { float l = FastMath.pow(FastMath.nextRandomFloat(), 1f / 3f); float u = FastMath.nextRandomFloat() * 2f - 1f; float o = FastMath.nextRandomFloat() * FastMath.TWO_PI; store.z = l * u; u = 1f / FastMath.fastInvSqrt(1f - u * u); store.x = l * u * FastMath.cos(o); store.y = l * u * FastMath.sin(o); store.multLocal(radius); store.addLocal(center); } public void getRandomPoint2(Vector3f store) { do { store.x = (FastMath.nextRandomFloat() * 2f - 1f); store.y = (FastMath.nextRandomFloat() * 2f - 1f); store.z = (FastMath.nextRandomFloat() * 2f - 1f); } while (store.lengthSquared() > 1); store.multLocal(radius); store.addLocal(center); } // Test public void testGetRandomPoint() { int n = 1000000; long start = System.nanoTime(); for (int i = 0; i < n; i++) { getRandomPoint1(store); } long time1 = System.nanoTime() - start; start = System.nanoTime(); for (int i = 0; i < n; i++) { getRandomPoint2(store); } long time2 = System.nanoTime() - start; System.out.println("t1:" + time1); System.out.println("t2:" + time2); System.out.println("t1/t2:" + (float) time1 / time2); } Result: t1:352272158 t2:94436324 t1/t2:3.7302613 Method2 seems nearly 4 times faster than method1.

jmecn · 2018-01-17T00:57:24Z

@Nehon sir I have conbined the commits, please check my code.

Nehon · 2018-01-17T06:59:02Z

Perfect, nice work.
I'll merge this PR once the build pass

jmecn · 2018-01-20T16:03:17Z

Why this pr build failed? any reason?

empirephoenix · 2018-01-20T19:35:43Z

Seems like an error with the buildserver (all buildlogs are null/empty) , I suggest to just commit a extra space or similar somewhere, so that it is triggered again.

Nehon · 2018-01-21T00:11:53Z

No don't commit again I'll relaunch the build

shamanDevel suggested changes Jan 14, 2018

View reviewed changes

shamanDevel mentioned this pull request Jan 14, 2018

Question: EmitterSphereShape.getRandomPoint #548

Closed

Nehon suggested changes Jan 14, 2018

View reviewed changes

Nehon approved these changes Jan 15, 2018

View reviewed changes

jmecn added 2 commits January 17, 2018 08:52

[taken]Fix issue #783

a025fb5

Fix issue #783

Nehon merged commit 1b2c84d into jMonkeyEngine:master Jan 21, 2018

Uh oh!

Conversation

jmecn commented Jan 14, 2018

Uh oh!

shamanDevel Jan 14, 2018

Choose a reason for hiding this comment

Uh oh!

Nehon Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TehLeo Jan 14, 2018

Choose a reason for hiding this comment

Uh oh!

Nehon Jan 14, 2018

Choose a reason for hiding this comment

Uh oh!

shamanDevel Jan 14, 2018

Choose a reason for hiding this comment

Uh oh!

TehLeo commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nehon left a comment

Choose a reason for hiding this comment

Uh oh!

Nehon Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmecn commented Jan 14, 2018

Uh oh!

TehLeo commented Jan 14, 2018

Uh oh!

jmecn commented Jan 14, 2018

Uh oh!

TehLeo commented Jan 14, 2018

Uh oh!

jmecn commented Jan 15, 2018

Uh oh!

Nehon commented Jan 15, 2018

Uh oh!

TehLeo commented Jan 15, 2018

Uh oh!

Nehon commented Jan 15, 2018

Uh oh!

jmecn commented Jan 16, 2018

Uh oh!

Nehon commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmecn commented Jan 17, 2018

Uh oh!

Nehon commented Jan 17, 2018

Uh oh!

jmecn commented Jan 20, 2018

Uh oh!

empirephoenix commented Jan 20, 2018

Uh oh!

Nehon commented Jan 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Nehon Jan 14, 2018 •

edited

Loading

TehLeo commented Jan 14, 2018 •

edited

Loading

Nehon Jan 14, 2018 •

edited

Loading

Nehon commented Jan 16, 2018 •

edited

Loading